-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bind all methods of Auth0VueClient to this #110
Conversation
src/plugin.ts
Outdated
const plugin = this; | ||
|
||
// Vue Plugins can have issues when passing around the instance to `provide` | ||
// Therefor we need to bind all methods correctly to `this`. | ||
this.loginWithRedirect = this.loginWithRedirect.bind(plugin); | ||
this.loginWithPopup = this.loginWithPopup.bind(plugin); | ||
this.logout = this.logout.bind(plugin); | ||
this.getAccessTokenSilently = this.getAccessTokenSilently.bind(plugin); | ||
this.getAccessTokenWithPopup = this.getAccessTokenWithPopup.bind(plugin); | ||
this.checkSession = this.checkSession.bind(plugin); | ||
this.handleRedirectCallback = this.handleRedirectCallback.bind(plugin); | ||
this.buildAuthorizeUrl = this.buildAuthorizeUrl.bind(plugin); | ||
this.buildLogoutUrl = this.buildLogoutUrl.bind(plugin); | ||
this.__checkSession = this.__checkSession.bind(plugin); | ||
this.__refreshState = this.__refreshState.bind(plugin); | ||
this.__proxy = this.__proxy.bind(plugin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question, is there a dynamic way we can do this (e.g. find all functions that are direct members of this
and bind it)? I can just see this being missed later; if we add new methods later we're likely to forget to add it to this list and run into issues when someone tries to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR to do it more dynamic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool
@@ -148,7 +153,7 @@ export class Auth0Plugin implements Auth0VueClient { | |||
} | |||
} | |||
|
|||
async __refreshState() { | |||
private async __refreshState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this private as it shouldnt be public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, although Browserstack tests are failing.
Also not sure about the FOSSA check though.
There seems to be an issue when setting
this
as the value when calling Vue'sprovide
. To solve it, we should bind all methods usingthis
.Vuex appeared to have solved it in the same way.